Address code review feedback: fix SQL injection, QueueMetrics bug, reduce boilerplate by Copilot · Pull Request #22 · jason810496/pgmq-sqlalchemy

Copilot · 2026-01-02T09:16:46Z

Description

Addresses three code review comments on PR #21:

1. Fixed SQL injection vulnerability in send operations

Replaced f-string query construction with parameterized queries in _get_send_statement and _get_send_batch_statement
Updated send, send_async, send_batch, send_batch_async to pass parameters to execute()

Before:

def _get_send_statement(queue_name: str, message: dict, delay: int) -> str:
    encoded_message = encode_dict_to_psql(message)
    return f"select * from pgmq.send('{queue_name}',{encoded_message},{delay});"

After:

def _get_send_statement(queue_name: str, message: dict, delay: int) -> Tuple[str, Dict[str, Any]]:
    return (
        "select * from pgmq.send(:queue_name, :message::jsonb, :delay);",
        {"queue_name": queue_name, "message": json.dumps(message), "delay": delay},
    )

2. Fixed QueueMetrics initialization error

Removed invalid scrape_time parameter from all metrics methods
QueueMetrics dataclass only defines 5 fields, was being called with 6

3. Reduced boilerplate in queue.py

Added _execute_operation helper to eliminate sync/async session management duplication
Refactored 7 methods: create_queue, create_partitioned_queue, validate_queue_name, drop_queue, list_queues, send, send_batch
Reduced ~150 lines of repetitive code

4. Added comprehensive tests for PGMQOperation

Created tests/test_operation.py with 18+ test cases covering sync and async static methods
Tests queue operations, message handling, metrics, and transaction control

Status

In progress
Ready for review
Done

Checklist

Read the Contributing Guide
Passes tests
Linted ( we use pre-commit with ruff )
Updated documentation

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: jason810496 <[email protected]>

* Add transation friendly op staticmethod class * Refactor PGMQueue methods to utilize PGMQOperation for extension checks, queue creation, validation, and message handling - Replaced direct SQL execution with calls to PGMQOperation methods for checking pgmq and pg_partman extensions. - Consolidated queue creation, validation, and message sending methods to use PGMQOperation for both sync and async operations. - Updated method signatures to include optional session and commit parameters for better session management. - Removed redundant sync and async methods, streamlining the codebase and improving maintainability. * Add empty test_operation * Move _validate_partition_interval to operations * Address code review feedback: fix SQL injection, QueueMetrics bug, reduce boilerplate (#22) * Initial plan * Fix SQL injection vulnerability and QueueMetrics bug in operation.py Co-authored-by: jason810496 <[email protected]> * Refactor queue.py to reduce boilerplate with _execute_operation helper Co-authored-by: jason810496 <[email protected]> * Add comprehensive tests for PGMQOperation class Co-authored-by: jason810496 <[email protected]> * Fix json import and send_batch parameter handling Co-authored-by: jason810496 <[email protected]> * Fix test exception handling to be more specific Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Improve test coverage for operation module and fix SQL parameterization bugs (#23) * Initial plan * Add comprehensive test coverage for PGMQOperation methods - Added tests for delete_batch (sync and async) - Added tests for archive_batch (sync and async) - Added tests for purge (sync and async) - Added tests for read_with_poll (sync and async) - Added tests for drop_queue (sync and async) - Added tests for check_pg_partman_ext (sync and async) - Added tests for create_partitioned_queue (sync and async) - Added tests for time-based partitioned queues Note: Tests currently fail due to SQL parameterization bug in operation.py where mixed parameter styles (:param and %(param)s) cause syntax errors. This was introduced in commit 849a8f1. Co-authored-by: jason810496 <[email protected]> * Fix SQL parameterization bugs and improve partition interval validation - Fixed SQL injection fix by using CAST(:param AS type) instead of :param::type - Fixed delete_batch and archive_batch to return message IDs instead of boolean - Improved partition interval validation to accept numeric strings - Added validation to sync create_partitioned_queue for consistency - Fixed queue name length in partitioned queue tests - All new tests now pass successfully Co-authored-by: jason810496 <[email protected]> * Fix send_batch array formatting and validate_queue_name test - Fixed send_batch to properly escape JSON in PostgreSQL array literal format - Fixed test_validate_queue_name to handle both ProgrammingError and InternalError - All 103 operation tests now pass successfully Co-authored-by: jason810496 <[email protected]> * Address code review feedback - Move imports to top of file (time, InternalError) - Add documentation comment explaining array literal escaping approach - All tests still passing Co-authored-by: jason810496 <[email protected]> * Improve import ordering and documentation - Fix import grouping (stdlib imports together) - Enhance documentation for array literal escaping with security notes - All tests still passing Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Fix PGMQueue with _execute_operation for rest of the methods * Fix Jsonb format * Fix CI to properly report test failures and refactor test fixtures (#24) * Initial plan * Fix CI configuration to properly report test failures Co-authored-by: jason810496 <[email protected]> * Refactor test fixtures to remove lazy fixtures and use dynamic parametrization Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Fix JSONB parameter binding to prevent psycopg2 dict adaptation errors (#25) * Initial plan * Fix JSONB handling with bindparams for send and send_batch operations Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Fix partition interval validation for negative numeric strings (#26) * Initial plan * Add read_archive methods to PGMQOperation and PGMQueue Co-authored-by: jason810496 <[email protected]> * Fix partition interval validation for negative numeric strings Co-authored-by: jason810496 <[email protected]> * Apply ruff formatting fixes Co-authored-by: jason810496 <[email protected]> * Remove read_archive methods and tests per spec clarification Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Add pytest-asyncio * Fix ambiguous column references in delete and archive SQL statements (#27) * Initial plan * Fix ambiguous column references in delete_batch and archive_batch Co-authored-by: jason810496 <[email protected]> * Add aliases to delete and archive statements to avoid ambiguity Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Fix ambiguous error manually Fix type hint * Fix bindparams usage Fix typ hint * Try fix text to varchar wrong cast error, remove drop_archive test * Fix text type casting * Fix sqlalchemy.exc.ArgumentError * Fix pgmq.delete_bactch and pgmq.archive_batch * Fix missing pgmq_all_variants fixture import in test_queue.py (#28) * Initial plan * Fix pgmq_all_variants fixture import in test_queue.py Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Improve test coverage for `op` module based on review feedback (#29) * Initial plan * Add tests for _execute_operation with provided session and read_with_poll without vt Co-authored-by: jason810496 <[email protected]> * Add test for async _execute_operation path when session is None Co-authored-by: jason810496 <[email protected]> * Remove fragile line number references from test comments Co-authored-by: jason810496 <[email protected]> * Clarify comment in test_read_with_poll_without_vt Co-authored-by: jason810496 <[email protected]> * Address final code review feedback: improve test structure and add cleanup verification Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: Copilot <[email protected]>

Initial plan

282df12

Copilot AI assigned Copilot and jason810496 Jan 2, 2026

Copilot AI mentioned this pull request Jan 2, 2026

Add op module for transaction #21

Merged

Copilot started work on behalf of jason810496 January 2, 2026 09:17 View session

Copilot AI and others added 5 commits January 2, 2026 09:21

Fix SQL injection vulnerability and QueueMetrics bug in operation.py

acba17d

Co-authored-by: jason810496 <[email protected]>

Refactor queue.py to reduce boilerplate with _execute_operation helper

afd8e34

Co-authored-by: jason810496 <[email protected]>

Add comprehensive tests for PGMQOperation class

adbd5af

Co-authored-by: jason810496 <[email protected]>

Fix json import and send_batch parameter handling

9ef221a

Co-authored-by: jason810496 <[email protected]>

Fix test exception handling to be more specific

ddabdaf

Co-authored-by: jason810496 <[email protected]>

Copilot AI changed the title ~~[WIP] Address feedback from code review on op module~~ Address code review feedback: fix SQL injection, QueueMetrics bug, reduce boilerplate Jan 2, 2026

Copilot AI requested a review from jason810496 January 2, 2026 09:30

Copilot finished work on behalf of jason810496 January 2, 2026 09:30

jason810496 marked this pull request as ready for review January 2, 2026 13:05

jason810496 merged commit 849a8f1 into feature/add-transaction-class Jan 2, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address code review feedback: fix SQL injection, QueueMetrics bug, reduce boilerplate#22

Address code review feedback: fix SQL injection, QueueMetrics bug, reduce boilerplate#22
jason810496 merged 6 commits intofeature/add-transaction-classfrom
copilot/sub-pr-21

Copilot AI commented Jan 2, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Copilot AI commented Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

1. Fixed SQL injection vulnerability in send operations

2. Fixed QueueMetrics initialization error

3. Reduced boilerplate in queue.py

4. Added comprehensive tests for PGMQOperation

Status

Checklist

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Copilot AI commented Jan 2, 2026 •

edited

Loading